Skip to content

test: add functional_dependencies.slt covering functional dependency driven optimizations - #23821

Merged
alamb merged 3 commits into
apache:mainfrom
alamb:alamb/fd-slt-coverage
Jul 24, 2026
Merged

test: add functional_dependencies.slt covering functional dependency driven optimizations#23821
alamb merged 3 commits into
apache:mainfrom
alamb:alamb/fd-slt-coverage

Conversation

@alamb

@alamb alamb commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

This PR adds test coverage rather than closing an issue. It documents the current behavior of these bugs so that fixing them shows up as a test change:

Rationale for this change

While reviewing #23636 @neilconway and I kept coming up with more examples of bad plans, and it was not obvious which of the surrounding wrong answers were pre-existing and which the PR introduced.

What changes are included in this PR?

A new datafusion/sqllogictest/test_files/functional_dependencies.slt with one section per consumer of functional dependencies:

  1. ReplaceDistinctWithAggregate — removing DISTINCT
  2. eliminate_duplicated_expr — dropping trailing ORDER BY keys
  3. optimize_projections — dropping GROUP BY expressions
  4. add_group_by_exprs_from_dependencies — selecting non-grouped columns
  5. GROUP BY derived keys on the NULL-padded side of an outer join

Cases that currently return wrong answers are labelled BUG with the expected
result and a link to the issue:

Case Symptom Issue
1.2 DISTINCT over a nullable UNIQUE column returns both NULL rows #23634
2.2 ORDER BY x, y drops the y key, so the NULL rows come back unordered #23818
3.2 GROUP BY x, y drops y, merging the two NULL groups and losing a row #23819
4.2 SELECT x, y ... GROUP BY x returns two rows for the x = NULL group #23820

Are these changes tested?

CI

Are there any user-facing changes?

No.

DataFusion derives functional dependencies from PRIMARY KEY / UNIQUE
constraints and from GROUP BY keys, and several optimizer rules consume
them. Coverage today is scattered through group_by.slt, which makes it hard
to tell which behaviors are correct and which are known bugs.

Add a dedicated test file with one section per consumer:

  1. ReplaceDistinctWithAggregate         (removing DISTINCT)
  2. eliminate_duplicated_expr            (dropping trailing ORDER BY keys)
  3. optimize_projections                 (dropping GROUP BY expressions)
  4. add_group_by_exprs_from_dependencies (selecting non-grouped columns)
  5. GROUP BY derived keys on the NULL-padded side of an outer join

Each section contrasts a non-nullable PRIMARY KEY against a nullable UNIQUE
column, since that is the distinction the consumers get wrong: SQL UNIQUE
permits multiple NULL rows, so a UNIQUE column is a key only among the
non-NULL rows.

The expected results record current behavior. Four cases produce wrong
answers today and are labelled BUG with the expected result and a link to
the issue tracking it:

  * 1.2 DISTINCT over a nullable UNIQUE column returns both NULL rows (apache#23634)
  * 2.2 ORDER BY x, y drops the `y` key, so the NULL rows come back
    unordered (apache#23818)
  * 3.2 GROUP BY x, y drops `y`, merging the two NULL groups and losing a
    row (apache#23819)
  * 4.2 SELECT x, y ... GROUP BY x returns two rows for the x = NULL
    group (apache#23820)

Only logical plans are shown, since all of these rules run during logical
optimization.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Jul 22, 2026
@alamb

alamb commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

It would be nice to add coverage for multi-column functional dependencies too, but I wanted to keep this PR small to make it eaiser to review

@codecov-commenter

codecov-commenter commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.71%. Comparing base (582453b) to head (8fdf2e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23821      +/-   ##
==========================================
- Coverage   80.72%   80.71%   -0.01%     
==========================================
  Files        1090     1090              
  Lines      370384   370384              
  Branches   370384   370384              
==========================================
- Hits       298975   298950      -25     
- Misses      53590    53610      +20     
- Partials    17819    17824       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb, maybe we can make comments little bit less verbose

@alamb

alamb commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @alamb, maybe we can make comments little bit less verbose

Thanks @comphead -- I actually cut down the comments quite a bit from the initial LLM output -- the current set I think explains what is going on. Given these are all bugs related to NULL corner cases (for example, I learned that UNIQUE allows duplicate NULL values!) I wanted to try and help future readers understand them if they started failnig

@alamb
alamb added this pull request to the merge queue Jul 24, 2026
@alamb
alamb removed this pull request from the merge queue due to a manual request Jul 24, 2026

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @comphead -- I took another pass and deleted a bunchmore commentary in 8fdf2e0


# These rules all run during logical optimization, so show only logical plans.
statement ok
set datafusion.explain.logical_plan_only = true;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only logical plans are shown, since all of these rules run during logical optimization.

@alamb
alamb enabled auto-merge July 24, 2026 15:50
@alamb
alamb added this pull request to the merge queue Jul 24, 2026
Merged via the queue into apache:main with commit efa84e8 Jul 24, 2026
37 checks passed
@alamb
alamb deleted the alamb/fd-slt-coverage branch July 24, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants